-
Notifications
You must be signed in to change notification settings - Fork 1
Simplyfing qubit.new
#518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplyfing qubit.new
#518
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
@weinbe58 I thought this was meant to replace the |
either way I need to put the |
TODO:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I just have some minor comments and questions.
FYI, I also checked to make sure that the examples in the documentation still work after this change.
new_func = func.Function( | ||
sym_name=sym_name, body=callable_region, signature=new_signature | ||
) | ||
mt_ = ir.Method(None, None, sym_name, [], mt.dialects, new_func) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit to being a bit lost here. Why is it necessary to construct a new method like this here? Why is mt_ = mt.similar(mt.dialects)
not sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature is different, I removed all the arguments of the function.
def get_measurement_id(measurement: MeasurementResult) -> int: ... | ||
|
||
|
||
# TODO: investigate why this is needed to get type inference to be correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth considering this as part of #549, since depending on the changes there, MeasureQubit
and MeasureQubitList
may be consolidated.
cc @johnzl-777
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is a bit old now isn't it? I'm all for getting this in first and then I can make the necessary tweaks in a dedicated PR for #549 .
The request for changes might take a bit considering Phillip's in Munich for the Munich Quantum Software Forum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment here is more on the Kirin side of things.
In principle the statements typing should just work as it but for some reason it wasn't so I added an explicit type inference method
Co-authored-by: David Plankensteiner <[email protected]>
Co-authored-by: David Plankensteiner <[email protected]>
…EraComputing/bloqade-circuit into phil/508-simplifying-qubitnew
def get_measurement_id(measurement: MeasurementResult) -> int: ... | ||
|
||
|
||
# TODO: investigate why this is needed to get type inference to be correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment here is more on the Kirin side of things.
In principle the statements typing should just work as it but for some reason it wasn't so I added an explicit type inference method
Closes #508 however I am not sure where to reexport
new
since I Can't reexport it into thequbit
module. For now I have re-exported it intosquin.new
.